Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Middleware for adding swagger ui endpoint (#2494) #2556

Merged
merged 2 commits into from
Dec 13, 2023

Conversation

987Nabil
Copy link
Contributor

Html template script rendering fix (#2521)

I also removed

  /**
   * Returns a new pattern that is extended with the specified segment pattern.
   */
  final def /[B](segment: SegmentCodec[B])(implicit combiner: Combiner[A, B]): PathCodec[combiner.Out] =
    self ++ Segment[B](segment)

Since it lead to code like Middleware.swaggerUI("docs" / "openapi", openAPI) being invalid.
Here "docs" / "openapi" was ambiguous, since there is a implicit conversion from string to PathCodec and to SegmentCodec and a / method for each of them. I saw that the SegmentCodec method was internally used only in one place. And I don't think that ppl will explicitly create SegmentCodec in there user code.
It seems reasonable to only have one impl of def /...

@987Nabil 987Nabil force-pushed the swagger-ui-middleware branch 3 times, most recently from a12d489 to 03d7325 Compare December 12, 2023 18:25
@codecov-commenter
Copy link

codecov-commenter commented Dec 12, 2023

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (9a9aef2) 64.59% compared to head (8155395) 64.84%.

Files Patch % Lines
...http/src/main/scala/zio/http/codec/PathCodec.scala 75.00% 1 Missing ⚠️
...in/scala/zio/http/endpoint/openapi/SwaggerUI.scala 96.55% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2556      +/-   ##
==========================================
+ Coverage   64.59%   64.84%   +0.25%     
==========================================
  Files         138      139       +1     
  Lines        8086     8118      +32     
  Branches     1459     1482      +23     
==========================================
+ Hits         5223     5264      +41     
+ Misses       2863     2854       -9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

* of the OpenAPI specification and is url encoded.
*/
def swaggerUI(path: PathCodec[Unit], api: OpenAPI, apis: OpenAPI*): Middleware[Any] =
swaggerUI(path, "5.10.3", api, apis: _*)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this hard-coded version here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is just a default version, that is the latest for now. I thought it might be nice to not have to look up a swagger ui version for a simple use case.

val routes = Routes(getUserRoute, getUserPostsRoute)
val openAPI = OpenAPIGen.fromEndpoints(title = "Endpoint Example", version = "1.0", getUser, getUserPosts)

val routes = Routes(getUserRoute, getUserPostsRoute) @@ Middleware.swaggerUI("docs" / "openapi", openAPI)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to ask the question why is this functionality a middleware? It is not modifying the behavior of existing routes just generates one or more new ones. Would not it be more logical if it is just a function returning Routes which can be appended to an existing Routes?

@987Nabil 987Nabil force-pushed the swagger-ui-middleware branch from 03d7325 to 8155395 Compare December 13, 2023 09:22
@987Nabil 987Nabil merged commit 61fa20b into zio:main Dec 13, 2023
14 checks passed
@987Nabil 987Nabil deleted the swagger-ui-middleware branch December 13, 2023 10:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants